System Hardening Phase 1: Catch errors at top level, take two#641
System Hardening Phase 1: Catch errors at top level, take two#641johnnyshields wants to merge 5 commits intorack:mainfrom
Conversation
…nd instead catches errors at the top level.
| configuration.tracked?(request) | ||
| @app.call(env) | ||
| end | ||
| rescue *allowed_errors |
There was a problem hiding this comment.
On second thought, wouldn't this also rescue dalli and redis errors ocurring inside other middlewares down the line called in @app.call(env)?
There was a problem hiding this comment.
You're right, I've refactored the scope which needs error handling into its own method.
|
@grzuy @santib Your help to review this PR is appreciated. @santib in previous threads you had proposed introducing an intermediate error such as "StoreProxyError". I've expressed that I don't feel there is a big advantage in doing this, as it obscures the underlying errors which have valuable information. As you'll see in my end-state I'm adding a hook to report errors e.g. via Sentry/Bugsnag (user adds their own), so keeping the raw Redis/Dalli errors are valuable. It would be great if we could merge this so I can proceed on the series of PRs that users are waiting on. If at the end there is a strong reason to introduce a StoreProxyError I'm happy to add it at the end. |
|
Hey @johnnyshields, I’d like to make clear that not necessarily all of the proposed PRs will get merged. We’ll need to evaluate the tradeoffs of each of them. Regarding this PR, I think the end goal it’s solving is valuable and would like to see it merged once we agree on the solution 👍 Regarding the solution, I still think the wrapper error is better (this wouldn’t have even happened), and maybe we should even consider going a step further and do something like this. The fact that we raise a custom rack attack error doesn't mean we can't provide the original error to devs wherever we want. But I’d like to hear @grzuy thoughts on this before moving on with either solution. |
|
@santib lets keep it simple. This PR has a low-complexity implementation. We can always introduce more complexity (new error classes, Exception.cause, etc) at anytime in the future if it is warranted. |
|
@santib can this be merged? |
3 participants
This PR removes the
rescuingblocks from Dalli/RedisProxy classes and instead catches errors at the top level.It is a simpler version of #639